Skip to content

Fix issue with SimplePhpView::create()#183

Open
DanielPlainview wants to merge 1 commit into
onPHP:masterfrom
DanielPlainview:simplePhpView
Open

Fix issue with SimplePhpView::create()#183
DanielPlainview wants to merge 1 commit into
onPHP:masterfrom
DanielPlainview:simplePhpView

Conversation

@DanielPlainview

Copy link
Copy Markdown
Contributor

В данный момент SimplePhpView::create() возвращает инстанс EmptyView. Данный PR позволяет исправить ситуацию таким образом, чтобы сохранить BC.

@stev

stev commented Mar 26, 2013

Copy link
Copy Markdown
Contributor

если проблема только в возврате EmptyView, через ReflectionClass это уж слишком (:
так не достаточно будет ?

public static function create()
{
    return new static();
}

@DanielPlainview

Copy link
Copy Markdown
Contributor Author

У них разные конструкторы.

@stev

stev commented Mar 26, 2013

Copy link
Copy Markdown
Contributor

а да. все равно похоже на хак
интерфейсы у них разные, и это попытка прилепить костыль сохраняя ВС.
по хорошему здесь не должно быть наследование SimplePhpView от EmptyView.

@DanielPlainview

Copy link
Copy Markdown
Contributor Author

Это не «похоже не хак», а хак и есть. Мне кажется, это очевидно. Я не очень улавливаю мысль, которую вы пытаетесь донести.

@stev

stev commented Mar 26, 2013

Copy link
Copy Markdown
Contributor
@@ -12,11 +12,16 @@
        /**
         * @ingroup Flow
        **/
-       class SimplePhpView extends EmptyView
+       class SimplePhpView implements View, Stringable
        {
                protected $templatePath         = null;
                protected $partViewResolver     = null;
-               
+
+               public static function create($templatePath, ViewResolver $partViewResolver)
+               {
+                       return new static($templatePath, $partViewResolver);
+               }
+
                public function __construct($templatePath, ViewResolver $partViewResolver)
                {
                        $this->templatePath = $templatePath;

@DanielPlainview

Copy link
Copy Markdown
Contributor Author

Мы с вами говорим на разных языках. Приведённый код всем очевиден. Его проблема в том, что он ломает BC, поэтому нужен хак.

@crazedr0m

Copy link
Copy Markdown
Contributor

Простите, ни как не могу увидеть, а чем ломатся BC?

@DanielPlainview

Copy link
Copy Markdown
Contributor Author
public function render(EmptyView $view)
{
    // ...
}

или какой-нибудь instanceof EmptyView.

@crazedr0m

Copy link
Copy Markdown
Contributor

ну я не знаю, на мой взгляд привязываться нужно или к SimplePhpView или к View
потому как SimplePhpView чаще всего далеко не empty

@DanielPlainview

Copy link
Copy Markdown
Contributor Author

@crazedr0m тут рассуждать особо нечего: да, наследоваться так было нельзя. Теперь желательно исправить ситуацию. Либо хаком и с BC, либо нормально.

@stev

stev commented Mar 26, 2013

Copy link
Copy Markdown
Contributor

public function render(EmptyView $view)

не нашел в коде фрейм-ворка похожего фрагмента.

@crazedr0m +1
не нашел поломки BC

@DanielPlainview

Copy link
Copy Markdown
Contributor Author

не нашел в коде фрейм-ворка похожего фрагмента

Вы правда надеялись найти это в коде фреймворка? Интересный вы человек. Я говорил про обычный код проекта, в котором используется onPHP.

@suquant

suquant commented Mar 26, 2013

Copy link
Copy Markdown
Member

:D
Предлагаю сделать другой класс нормальный а этот сделать deprecated

@stev

stev commented Mar 26, 2013

Copy link
Copy Markdown
Contributor

Уважаемый @DanielPlainview - понаписать у себя в проекте можно все что угодно.
Это не ваш личный ВС.
Поддерживать то что каждый "кулибин" у себя написал, в рамках фреймворка - это утопия.
Ситуация может быть другой если львиная доля участников использует данный кусок.
Посмотрим что скажут остальные.

@AlexeyDsov

Copy link
Copy Markdown
Member

При изменении наследования BC ломается, если кто-то вдруг проверяет instanceof EmptyView

@dovg

dovg commented Apr 1, 2013

Copy link
Copy Markdown
Member

@stev мушку спили.

По существу

Предлагаю сделать другой класс нормальный а этот сделать deprecated

+1

Во времена Voxus bc ломали только в путь (со сменой версии), ничего страшного в этом нет. Если тянуть совместимость вечно, то получится винда какая-то.

@AlexeyDsov

Copy link
Copy Markdown
Member

Впринципе почемы бы и не сломать здесь BC, убрав из цепочки наследования EmptyView. Делать deprecated и потом переименовывать в проекте класс на другой будет сложнее чем пробежаться по использованию EmptyView которого должно быть не много или вообще не быть.

@pupkinV

pupkinV commented Apr 2, 2013

Copy link
Copy Markdown
Contributor

@dovg святые слова. очень часто здесь употребляется BC и даже тогда, когда оно является Bug Compatibility. Хотя в данном случае действительно изменяется сигнатура метода.

@stev

stev commented Apr 13, 2013

Copy link
Copy Markdown
Contributor

для 1.0 - deprecated,
для 1.1 - я за поломку ВС (Bug Compatibility) :)

есть еще кроме нас кто работает на прод. с 1.1 ?

PS. и да, если вдруг слова про "Кулибина" могли быть восприняты как-то негативно, то это не в таком ключе говорилось. Кулибин - выдающийся человек, исследователь и изобретатель, и каждого из нас можно в какой-то степени соотнести к нему. (ну мало ли как могут слова быть интерпретированы)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants